Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cherry pick from "next" branch #45

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

gene1wood
Copy link
Collaborator

@gene1wood gene1wood commented Apr 13, 2018

This is a hand review of the 18 commits in the #44 next branch. The commits are a combination of

  • code that made it into master through another means
  • code that we don't want from next
  • code that we may want from next that I've rendered into 7 new commits. Some of these have been reformulated to work with the current codebase or modified to strip out things which move us farther from PEP008

We should review the functionality in this PR and if it's desired, merge it and throw away #44 and the next branch.

  • Ignoring 996e610 as it moves away from PEP008
  • Ignoring 6c77aab as it was added to master in f835846
  • Ignoring 29d4f14 as it's a version bump
  • Rendering 54b4b08 in new PR as d7575dc : Spelling fix
  • Rendering 079976b in new PR as 2ed2abe : Refactoring duplicate code so it's called once
  • Ignoring 90da9d2 as it is a change to the order of future planned work in the changelog
  • Ignoring 0a3f478 as it is planned work in v 2.0 in the Changelog
  • Ignoring 958ceee as it was added to master in 731b612 with a small exception of one variable name which I'm gonna leave
  • Ignoring 7697909 as it was added to master in cbb0288
  • Ignoring 28f5a31 as it was added to master in 7c02d0c
  • Ignoring ff6c0e8 as it was added to master in 8a44420
  • Ignoring f0f09ac as it was added to master in ea6191a
  • Rendering fad02fa in a new PR as 90c69b8 : Spelling fix
  • Rendering 51f5b78 in a new PR as 2db5876 : add line to make test.py executable
  • Rendering 6a44c59 in a new PR as 5f61c3e : Changelog addition
  • Ignoring e9cd78d as it was added to master in a84c16f
  • Rendering 3d37343 in a new PR as 74b49d2 : Add a media-type handler for XML
  • Rendering e1143ed in a new PR as 0706cfe : Bugfix

In the Client class:
    setConnectionProperties()
        - Rely on _fix_headers to fix the case of default headers at the time of request instead of duplicating its code

    _fix_headers()
        - When we addin default headers, ensure the case is correct

This is a reworking of 079976b from the `next` branch which accomplishes this differently
This is a reworking of fad02fa from the `next` branch. It removes a docstring change that moves away from PEP008
This is a from 51f5b78 from the `next` branch.
    - TODO: Reuse TCP connections
    - Update "Parse Content-Type" bullet

This adds new content to v2.0 about parsing Content-Type headers correctly. v2.0 came out in the past but I see the ctypeParameters dict it mentions.

This is a from 6a44c59 from the `next` branch.
We use xml.dom.minidom to de-serialize the results.

This is a from 3d37343 from the `next` branch.
…headers

In case the API does not supply extra_headers, self.request() will fail
trying to delete the 'content-type' header from them. Including the
_default_headers (always) ensures that 'content-type' is defined

This is a from e1143ed from the `next` branch.
@gene1wood gene1wood mentioned this pull request Apr 13, 2018
# Conflicts:
#	agithub/GitHub.py
#	agithub/base.py
#	agithub/test.py
@gene1wood
Copy link
Collaborator Author

So the only interesting stuff in this PR, and what we should probably look at to decide if we want to merge this is

@nnja Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant